-
Notifications
You must be signed in to change notification settings - Fork 0
chore: get_lca for NodeIndex #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
aab72c9
to
bc39814
Compare
bc39814
to
4f3409b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## tzahi/node_index/tight_range_checks #98 +/- ##
=======================================================================
+ Coverage 75.94% 76.38% +0.43%
=======================================================================
Files 25 25
Lines 869 885 +16
Branches 869 885 +16
=======================================================================
+ Hits 660 676 +16
Misses 175 175
Partials 34 34 ☔ View full report in Codecov by Sentry. |
c359279
to
bcabc7f
Compare
35502bb
to
489e2b0
Compare
e142824
to
c59efaf
Compare
489e2b0
to
e7358dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 6 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)
crates/committer/Cargo.toml
line 15 at r2 (raw file):
pretty_assertions.workspace = true rstest.workspace = true rand.workspace = true
Suggestion:
pretty_assertions.workspace = true
rand.workspace = true
rstest.workspace = true
crates/committer/src/patricia_merkle_tree/types.rs
line 83 at r2 (raw file):
Ordering::Greater => adapted_self = adapted_self >> (bit_length - other_bit_length), Ordering::Equal => return *self, }
clearer?
also removes the need for Ordering
.
if you like minimal amount of lines you can replace the if .. else
with match .. true => .. false => ..
Suggestion:
pub(crate) fn get_lca(&self, other: &NodeIndex) -> NodeIndex {
if self == other {
return *self;
}
let bit_length = self.bit_length();
let other_bit_length = other.bit_length();
// Bring self to the level of other.
let adapted_self = if self < other {
self << (other_bit_length - bit_length)
} else {
self >> (bit_length - other_bit_length)
};
crates/committer/src/patricia_merkle_tree/types.rs
line 86 at r2 (raw file):
let xor = adapted_self.0 ^ other.0; // The length of the reminder after removing the common prefix of the two nodes.
typo
Suggestion:
remainder
1fd20f3
to
5f7259d
Compare
40447a5
to
97bf4b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @dorimedini-starkware)
crates/committer/Cargo.toml
line 15 at r2 (raw file):
pretty_assertions.workspace = true rstest.workspace = true rand.workspace = true
Done.
crates/committer/src/patricia_merkle_tree/types.rs
line 83 at r2 (raw file):
Previously, dorimedini-starkware wrote…
clearer?
also removes the need forOrdering
.
if you like minimal amount of lines you can replace theif .. else
withmatch .. true => .. false => ..
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)
5f7259d
to
9fa2140
Compare
97bf4b9
to
1743662
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)
1743662
to
b1e87fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)
This change is